[firtool] Add option to disable layer sink #7981
Conversation
include/circt/Firtool/Firtool.h
Outdated
There was a problem hiding this comment.
Even though this is short lived, should this include updates to the C-API?
There was a problem hiding this comment.
I don't think the CAPI has been kept up to date properly, but I can try to figure this out.
lib/Firtool/Firtool.cpp
Outdated
| if (opt.shouldDisableLayerSink()) { | ||
| if (opt.shouldAdvancedLayerSink()) | ||
| pm.nest<firrtl::CircuitOp>().addPass( | ||
| firrtl::createAdvancedLayerSinkPass()); | ||
| else | ||
| pm.nest<firrtl::CircuitOp>().nest<firrtl::FModuleOp>().addPass( | ||
| firrtl::createLayerSinkPass()); | ||
| } |
There was a problem hiding this comment.
Given that we now have two options, it doesn't seem great that ALS is gated by another option. I.e., -advanced-layer-sink has no effect if -disable-layer-sink is set.
Would it be better to instead just make two simple options:
-advanced-layer-sink: schedules advanced layer sink-layer-sink: schedules layer sink
Alternatively an enum that has the three actually interesting states: -layer-sink=none,simple,advanced.
There was a problem hiding this comment.
I figured since we're going to fully replace LS with ALS and removing --advanced-layer-sink, that this would have the least churn. Then, we could keep the disable-layer-sink flag as long as we find it useful. Moving to an enum would have to be reversed when we remove either flag. I'm not sure about the first idea, I guess we would have to disable layersink by default to make it work?
There was a problem hiding this comment.
Yeah, it would have to be default of LS on and ALS off if there are two options.
(This isn't blocking in any way. Whatever you decide is fine here.)
youngar
force-pushed
the
firtool-disable-layersink
branch
from
1ddaf68 to
43d7963
Compare
The layer sink pass is showing some performance problems, and we want the ability to turn it off to work around the issue, until we can properly fix it.
youngar
force-pushed
the
firtool-disable-layersink
branch
from
43d7963 to
f1582ed
Compare
|
LGTM! Just a thought, maybe disable layer sink entirely if it's not used presently and problematic (until ALS is enabled, which already has an option)? |
The layer sink pass is showing some performance problems, and we want the ability to turn it off to work around the issue, until we can properly fix it.